-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add test coverage for SAT-28860 #17153
Add test coverage for SAT-28860 #17153
Conversation
trigger: test-robottelo |
PRT Result
|
trigger: test-robottelo |
PRT Result
|
PRT failed in
|
@vsedmik yes, it'll be fixed in Katello/katello#11266 |
66dd68e
to
c904a05
Compare
trigger: test-robottelo |
PRT Result
|
|
PRT Result
|
c904a05
to
9c69b44
Compare
trigger: test-robottelo |
PRT Result
|
trigger: test-robottelo |
PRT Result
|
9c69b44
to
18a5dd0
Compare
|
PRT Result
|
18a5dd0
to
e25e798
Compare
|
PRT Result
|
e25e798
to
e8933f5
Compare
|
PRT Result
|
|
PRT Result
|
e8933f5
to
0f787ae
Compare
|
PRT Result
|
Signed-off-by: Gaurav Talreja <[email protected]>
Signed-off-by: Gaurav Talreja <[email protected]>
0f787ae
to
c72446b
Compare
|
PRT Result
|
The remaining PRT failures are unrelated to the changes, so this PR is now RFR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
default_proxy = target_sat.api.Setting().search( | ||
query={'search': 'name=content_default_http_proxy'} | ||
)[0] | ||
assert default_proxy.value != http_proxy_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a good practice to do assertions in teardown. Can we handle assertions in the test and keep the cleanup part separate? The results are misleading when debugging failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so it'll mislead us while debugging any failures, instead I think teardowns are best to ensure the Satellite we're using is in the same state as it was before, and adding assertions there, ensures everything worked in setup and teardown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just one question I would like to clear out.
@@ -47,6 +48,7 @@ def make_http_proxy(self, org, http_proxy_type): | |||
username=settings.http_proxy.username, | |||
password=settings.http_proxy.password, | |||
organization=[org.id], | |||
content_default_http_proxy=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we want to always do this on proxy create via api_factory.make_http_proxy()
, this fixture never done this before since that option is new. What is the reason to do so now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vsedmik Yes, the fixture behaviour is still same, if we make it non-default then we had to configure the setting explicitely and that step we have removed it from this setup fixture, and this new option configures the setting on create
Problem Statement
Missing test coverage for new feature SAT-28860
Solution
Add test coverage for new feature SAT-28860
Related Issues
SatelliteQE/airgun#1636
SatelliteQE/nailgun#1254